refactor: use destructured defaults over per-field ?? fallbacks - #248
Merged
Conversation
Bring options-bag default handling into compliance with the AGENTS.md
convention 'Destructured defaults over ?? fallbacks': apply defaults to
a function's input surface via a single destructuring assignment instead
of per-field `??`.
- annotation/useSubmitOnCmdEnter.test.ts: buildKeyEvent destructures init
- cli/testHelpers/annotationFakes.ts: createAnnotationDependencies
destructures { submission, result } with defaults
- cli/updater/UpdaterImpl.ts: fold ttl default into the existing
this.deps destructure
No behavior change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
The repository's
AGENTS.mdstates a clear convention:I scanned every
??usage across all packages and grouped them. Most are legitimate null-coalescing (Map.get fallbacks, optional-chaining results, array indexing, runtime object props) and are not violations. The violation class here is specifically per-field??defaulting on a function's options/deps bag. I fixed every instance:packages/annotation/src/useSubmitOnCmdEnter.test.ts—buildKeyEventappliedinit.metaKey ?? false,init.ctrlKey ?? false,init.preventDefault ?? (() => {}). Now destructuresinitwith defaults in one place.packages/cli/src/testHelpers/annotationFakes.ts—createAnnotationDependenciesusedoptions.submission ?? annotationSubmission.build()andoptions.result ?? Promise.resolve(submission). Nowconst { submission = annotationSubmission.build(), result = Promise.resolve(submission) } = options;.packages/cli/src/updater/UpdaterImpl.ts—checkForUpdatealready destructuredthis.depsandoptions, but reached back throughthis.deps.ttl ?? Temporal.Duration.from(...)on a separate line. Foldedttlinto the existingthis.depsdestructure with its default.No runtime behavior changes — these are pure structural refactors. In
annotationFakes.ts, movingresult's default into the destructure evaluatesPromise.resolve(submission)eagerly instead of inside thestartReviewServerclosure; sincesubmissionis already resolved at that point, the resulting settled promise is equivalent.Verification
tsctypecheck: pass for@contextbridge/cliand@contextbridge/annotation.prettier --check: pass on all three files.eslint --max-warnings 0: pass on all three files.bun testforUpdaterImpl.test.ts,plan.test.ts,open.test.ts(which exercisecreateAnnotationDependenciesandUpdaterImpl): 57 pass, 0 fail.annotationpackage uses vitest browser mode, which cannot launch in this sandbox (Chromium/Playwright not installed —browserType.launch: Executable doesn't exist). This is a pre-existing environment limitation unrelated to the change; the package typechecks cleanly and the edit is a behavior-identical test-helper refactor.Deliberately not changed (edge case)
packages/context/src/frontend.tshasconst logger = loggerOverride ?? createLogger({ transmit: telemetry.pinoTransmit });. This is not a violation to fix: the input is already destructured (logger: loggerOverride), and the default cannot be inlined into the destructure because it depends ontelemetry, which is computed later in the function body. Inlining it would force eager logger creation and change behavior.Other violation classes observed (for future runs)
createDeferredin a test file —packages/instrumentation/src/node/harnessDiscovery.test.tsdefines a localcreateDeferredwith anew Promisewrapper, which the testing-patterns rule explicitly prohibits (should import from@contextbridge/shared/testHelpers). Single instance; left for a focused follow-up.toMatchObjectfor structured payloads — several test files use runs of field-by-fieldexpect(obj.a).toBe(...)where onetoMatchObjectwould fit. Fuzzy (the rule allows separate assertions for orthogonal behavior / specialized matchers), so risky to bulk-apply mechanically.readFileSync/writeFileSync/readdirSyncwhereBun.file/Bun.write/Bun.Globare preferred, but the rule explicitly says these are "not banned outright," so it's a soft convention.